-
Notifications
You must be signed in to change notification settings - Fork 492
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a max_dust_htlc_exposure_msat
#919
Conversation
02-peer-protocol.md
Outdated
- if the `dust_balance_on_counterparty_tx` at the new `dust_buffer_feerate` is superior to `max_dust_htlc_exposure_msat`: | ||
- MAY NOT send `update_fee` | ||
- MAY fail the channel | ||
- if the `dust_balance_on_holder_tx` at the new `dust_buffer_feerate` is superior to the `max_dust_htlc_exposure_msat`: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be
- if the `dust_balance_on_holder_tx` at the new `dust_buffer_feerate` is superior to the `max_dust_htlc_exposure_msat`: | |
- if the `dust_balance_on_holder_tx` at the new `feerate_per_kw` is superior to the `max_dust_htlc_exposure_msat`: |
The real question is if the new feerate pushes the balance over the exposure limit, not if the new feerate + buffer does. Pushing a feerate update where the new buffer ceiling is above the current should just fail to add any new dusty htlcs, but will leave the existing ones without a problem. Otherwise what was the point of having the buffer in the first place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you're right that's the spec doesn't match our LDK code on that: https://github.com/rust-bitcoin/rust-lightning/blob/7aa2caccd884cd7d40ee146323c2a65b7ea39407/lightning/src/ln/channel.rs#L3130
Pushing a feerate update where the new buffer ceiling is above the current should just fail to add any new dusty htlcs, but will leave the existing ones without a problem.
Yes that's right. If the inbound/outbound feerate is above the the dust buffer feerate ceiling, the dust exposure on counterparty and holder commitment transactions should be evaluated. If one of them is above the dust HTLC exposure the channel could be fail (I think a MAY is better).
We had multiple back-and-forth on the LDK-side about the design of update_fee
reception mitigations.
Updating the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated at 6a1cb21. If you think there are more corrections needed, let me know.
To mitigate this scenario, a `max_dust_htlc_exposure_msat` must be apply at | ||
HTLC sending, forwarding and receiving. | ||
|
||
A node: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo this whole section should be moved into the update_add_htlc
requirements, as they're actions that are triggered by the receipt of this message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted, I'll update this PR in that sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, in fact it's not exactly clear that it should be in the update_add_htlc
requirements as it's a relay requirement, to me in the same category of cltv_expiry_delta
selection. At least was my thinking when I wrote those lines.
Though it can be a source of confusion and maybe we should even add a new BOLT4 message. Due to lack of that, I remember it was hard to assess that max_dust_htlc_exposure_msat
was well in-place when I verified fixes against Eclair in a black-box way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes but you don't enact these spec requirements until a update_add_htlc
message is received.
02-peer-protocol.md
Outdated
@@ -1127,6 +1190,17 @@ A receiving node: | |||
current commitment transaction: | |||
- SHOULD fail the channel, | |||
- but MAY delay this check until the `update_fee` is committed. | |||
- if the `dust_balance_on_counterparty_tx` at the new `dust_buffer_feerate` is superior to `max_dust_htlc_exposure_msat`: | |||
- MAY fail the channel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- MAY fail the channel | |
- SHOULD fail the channel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the rational just under. That's a MAY as automatic-closure might bring newer issues. A third-party to a channel could relay and withhold HTLCs across the link to provoke a close.
See also Eclair new config settings close-on-update-fee-dust-exposure-overflow
: https://github.com/ACINQ/eclair/pull/1985/files, which is false by default. Though in LDK we do auto-close by default.
As it's arbitrating between two safety risks, I think it's wiser to defer the choice to the implementator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well 'SHOULD' is still a deferral to the implementor, just a more strongly worded form!
02-peer-protocol.md
Outdated
- if the `dust_balance_on_counterparty_tx` at the new `dust_buffer_feerate` is superior to `max_dust_htlc_exposure_msat`: | ||
- MAY fail the channel | ||
- if the `dust_balance_on_holder_tx` at the new `dust_buffer_feerate` is superior to the `max_dust_htlc_exposure_msat`: | ||
- MAY fail the channel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- MAY fail the channel | |
- SHOULD fail the channel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #919 (comment)
And update some behavior to check both sides on receipt of a update_fee, as per the proposed spec. lightning/bolts#919
And update some behavior to check both sides on receipt of a update_fee, as per the proposed spec. lightning/bolts#919
c5b7b89
to
6a1cb21
Compare
02-peer-protocol.md
Outdated
- SHOULD NOT send this HTLC | ||
- SHOULD fail this HTLC if it's forwarded | ||
|
||
`dust_buffer_feerate` is defined as the maximum of either 2530 sats per kWU or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the spec needs to be this prescriptive. This is what we do in LDK, but others can/should do other things. I think we the spec should just talk generally about feerate_per_kw
and note that in calculating you SHOULD provide some buffer for future feerate increases.
And update some behavior to check both sides on receipt of a update_fee, as per the proposed spec. lightning/bolts#919
And update some behavior to check both sides on receipt of a update_fee, as per the proposed spec. lightning/bolts#919
And update some behavior to check both sides on receipt of a update_fee, as per the proposed spec. lightning/bolts#919
I don't think any implementer reading this will know how to implement it; it is too loose on requirements, and risks channel breakage as a result. Ideally, Alice wants to set She can, however, limit how much Bob can send. So this should simply say:
Now, this amount has not been defined anywhere! I am reluctant to add YA random value send on connection (and it won't help existing deployments), so perhaps we define it as 0.5% of the channel capacity, and gate it on a feature bit? This may make tiny channels unusable: if we care, the language above should be changed to always allow a single dust HTLC. |
Having a uniform dust threshold for both peers seems strictly nicer imo than having it be arbitrarily selected per peer; in fact the newly proposed set of rules from @rustyrussell makes it such that you'd need to know what the peer's dust threshold is so as to not exceed it when deciding whether to send an This points to the fact that interop is a bit problematic here given that ariard’s rules have already shipped in a few clients; if our peer is using ariard’s rules and has chosen a dust theshold that’s higher than ours, we’ll fail the channel/close the connection when they send an “ok for their dust threshold, but not for ours” htlc; this value is currently arbitrarily set by the peer. Given that Rusty's proposal uses an implicit (0.5% of chan balance or thereabouts) variable for 'consensus' so to speak, am I right in thinking we'd need a feature flag and some way to migrate existing channels over to this policy? It's going to be a lot slower to roll out if that's the case. |
Yes having to arbiter between "channel breakage" and "balance burning" risks have been laid out in the
Yes, also a peer can batch dust add. As such, the
I think the checks should be done on both peer and node's commitment transactions ? If the peer's
As a downside, a "consenus" value doesn't encompass node operators subjective loss threshold and the node's channels topology. E.g, if you have a high-degree of confidence in the peer's reliability and trustworthiness you might accept double-digits of the channel capacity as a If we want to account node operators risk preferences better, of course we can announce this new value at channel opening ? Though, only work for future deployment, in the lack of dynamic upgrades. |
It's not only tiny channels, if the feerate rises too much it becomes a real issue for a lot of channels. Unless we add a new rule as you suggest to allow at least one HTLC. However, since we're all migrating to I think this parameter really should be configurable by each node operator based on their risk aversion and their channel size. I don't think it's worth communicating to your peers either, since it's harmless when they send you HTLCs that overflow your tolerance, they simply end up being failed. And I do see node operators updating this value over time (maybe as they build trust with their peer) which means it would need to allow updates, which is a pain. |
02-peer-protocol.md
Outdated
|
||
A node: | ||
- upon an incoming HTLC: | ||
- if a HTLC's `amount_msat` is inferior to the counterparty's `dust_limit_satoshis` plus the HTLC-timeout fee at the `dust_buffer_feerate`: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, that's a formatting issue! Should be better now :)
And update some behavior to check both sides on receipt of a update_fee, as per the proposed spec. lightning/bolts#919
And update some behavior to check both sides on receipt of a update_fee, as per the proposed spec. lightning/bolts#919
And update some behavior to check both sides on receipt of a update_fee, as per the proposed spec. lightning/bolts#919
6a1cb21
to
37f1540
Compare
219adca
to
7705356
Compare
Updated at 7705356. If we think there is no more value in adding a clarification in the specification about dust exposure, I can close it. On the other-side, while looking on the validating lightning signer code recently, it turns out to be affected by the same security issue (https://lists.linuxfoundation.org/pipermail/lightning-dev/2023-January/003829.html) so I wouldn't be surprised people writing that type of auxiliary lightning softwares to screw up again, if not warned. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 7705356
I do think there is value in merging this PR since:
- this is a non-trivial issue that implementers should know about: we help new protocol developers ramp up more quickly by mentioning this explicitly in the spec
- we've been able to reduce this to a small enough change that it doesn't add too much clutter to the existing spec
But I believe we should have an ACK from cln
or lnd
to move forward with adding that change.
I’ll close this PR. Like said I know where are the flaws, not my issue if the wider ecosystem forgets about them. |
This was brought up in the latest spec meeting (re better go-to-chain heuristics factoring in chain unit economics): #1082 |
Good I think this is just missing one more review to be merged on top of the one of t-bast. On the long-term, I think transaction-relay policy rules could be relaxed to have dust outputs being spend in a single package a la ephemeral anchor. |
Spec meeting notes (#1098): pending review from @rustyrussell |
There are some nits I could pick (the naming makes it sound like a field on the wire, rather than an internal threshold), but it's basically sound. I like that it has fewer requirements with zero-fee-anchors, too. Ack 7705356 |
If you have specific nits feel free to let them, I’ll address them or please add them with a follow-up PR. Happy to add the recommendations we’re working for fee-bumping reserves management post-anchor we’re working for LDK as other spec recommendations, once we’re good with dust exposure here. |
Forgive me if I'm missing something, but this doesn't seem like a protocol change so much as a policy recommendation? The only thing I can imagine leaking into the protocol would be an error message that basically said it was overallocated on htlc exposure but I would imagine that would be (and should be) classified as a temp channel failure. If it is a policy rec, I would like to second what @rustyrussell said and at least "unquote" the term, if not move it to a different section. That said, I do see how we already have precedence for this with the " |
Note here the distinction between mechanism vs policy is maintained, as there is no mandatory selection of the value of Such authorization mechanism deployment alters the protocol behavior, and therefore qualifies as protocol change from my viewpoint. This is unclear to me what you qualify as a “pure” protocol change, as you note historically in the BOLT we have documented some variables selection, e.g you have other example such as fragmentation of punishment transaction post-anchor: https://github.com/lightning/bolts/blob/master/05-onchain.md#rationale-6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 7705356
* Bound exposure to trimmed in-flight HTLCs * Reject update_fee beyond max_dust_htlc_exposure_msat Co-authored-by: t-bast <[email protected]>
* Bound exposure to trimmed in-flight HTLCs * Reject update_fee beyond max_dust_htlc_exposure_msat Co-authored-by: t-bast <[email protected]>
No description provided.